-
Notifications
You must be signed in to change notification settings - Fork 173
feat(c/driver_manager): add connection profile interface #3876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
CC @davidhcoe if there's anyone I missed please feel free to tag others that might be interested in looking this over and reviewing it. Once everyone is onboard with the design and ideas I'll implement the handling of these profiles for the other language bindings. |
paleolimbot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
All just questions from me to take or leave 🙂
| /// version = 1 | ||
| /// driver = "driver_name" | ||
| /// | ||
| /// [options] | ||
| /// option1 = "value1" | ||
| /// option2 = 42 | ||
| /// option3 = 3.14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to wrap this in a top level [profile] item (like pyproject.toml's [project] or Cargo.toml's [workspace])?
| /// version = 1 | |
| /// driver = "driver_name" | |
| /// | |
| /// [options] | |
| /// option1 = "value1" | |
| /// option2 = 42 | |
| /// option3 = 3.14 | |
| /// [adbc.profile] | |
| /// version = 1 | |
| /// driver = "driver_name" | |
| /// | |
| /// [options] | |
| /// option1 = "value1" | |
| /// option2 = 42 | |
| /// option3 = 3.14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I was trying to avoid having to do that. Is there any benefit to doing so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just seemed like a common idiom in .toml configs, perhaps to allow for future expansion (e.g., if you ever want >1 profile to live in one .toml)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having multiple connection profiles defined in a single TOML file could potentially be nice, but it complicates the profile search procedure, and I worry that it could confuse users.
| AdbcStatusCode (*GetDriverName)(struct AdbcConnectionProfile* profile, | ||
| const char** driver_name, struct AdbcError* error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| AdbcStatusCode (*GetDriverName)(struct AdbcConnectionProfile* profile, | |
| const char** driver_name, struct AdbcError* error); | |
| AdbcStatusCode (*GetDriverName)(struct AdbcConnectionProfile* profile, | |
| const char** driver_name, AdbcDriverInitFunc* init_func, struct AdbcError* error); |
Is there any value to allowing a profile to optionally specify the init function directly? (R could maybe use this to find R package versions of drivers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way the code is set up doesn't allow for this. If there is an init func set then the profile handling is currently entirely skipped and it's up to the init_func to handle everything. Given the way we envision profiles working, I don't think that it makes sense for the profile to set or specify the init function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'd just set args->init_func instead of args->driver (understood if you'd prefer not to)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer not, but I'm open to it if others think it's worthwhile.
|
@CurtHagenlocher will be interested in this as well. |
| Overview | ||
| ======== | ||
|
|
||
| Similar to ODBC's ``odbc.ini``, the ADBC driver manager supports **connection profiles** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but I think we should explain this as its own thing, and cite ODBC as an inspiration further down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. How about we structure the beginning of this page similar to how the ADBC Driver Manager and Manifests docs page is structured? Like:
There are two ways to pass connection options to driver managers:
- Directly specifying connection options as arguments to driver manager functions in your application code.
- Referring to a connection profile which contains connection options.
(or something like that)
And we could add a section that briefly explains method 1, before continuing to the content here which covers method 2.
| driver = "adbc_driver_snowflake" | ||
| [options] | ||
| adbc.snowflake.sql.account = "env_var(SNOWFLAKE_ACCOUNT)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Why not use option = { env_var = "NAME" }?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would prevent us from using environment variables to define substrings, which we will need for the drivers that expect their connection arguments to be in a URI.
uri = "scheme://host:port?user=user&pw=env_var(PASSWORD)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider using ${} instead of env_var()?
That is more concise and makes escaping literals easier (e.g. with \$ or $$).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ${} might be a little more familiar, unless you expect to eventually have other things besides env_var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the expectation was that we would eventually have more special functions in addition to env_var which is why this was the original suggestion. For eventual consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further consideration, I think env_var is good, because it's clear and explicit, and because (as Matt says) it will be more consistent with future functions that we might want to add, for example to retrieve credentials from a secrets management tool.
| /// \brief Release the profile and perform any cleanup. | ||
| void (*release)(struct AdbcConnectionProfile* profile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Does the manager call release or does the application?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manager calls release currently, in general the application will never interact with profile objects unless they explicitly want to.
|
@CurtHagenlocher @davidhcoe @lidavidm @ianmcook any further comments? |
| AdbcDatabase database; | ||
| AdbcDatabaseNew(&database, &error); | ||
| AdbcDatabaseSetOption(&database, "profile", "my_snowflake_prod", &error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidavidm Similar to what I added in af734d3, should we also add a warning like this somewhere in the docs?
Driver managers may treat some option keys as manager-reserved and handle them without forwarding them to the underlying driver. In particular, the option key
profileis reserved for connection profiles and must not be implemented or interpreted by drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the best place to add this, @lidavidm ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the docstring for AdbcDatabaseSetOption in adbc.h is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 7b8d420.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidavidm Does this comment qualify as a sufficiently small change to adbc.h that this would not be considered a change to the ADBC API spec? I hope so.
An attempt to implement the ideas as suggested in #3765 to provide the ability to specify and load connection profiles to define options for use by the driver manager.
This creates an
AdbcConnectionProfilestruct and defines anAdbcConnectionProfileProviderfunction pointer typedef to allow for customized management of profiles. This also implements a default file-based profile provider as described in #3765 (comment) which will be used if no custom provider has been set.This allows easy expansion in the future for non-file-based connection profile providers while still implementing the easier case of using file-based profiles, including hierarchical specification for now. See the documentation comments added to
adbc_driver_manager.hfor the full description of the semantics and explanation.